-
Notifications
You must be signed in to change notification settings - Fork 751
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add some decoration to tool CLI #4865
Conversation
writeln!( | ||
printer.stderr(), | ||
"Uninstalled: {}", | ||
"Uninstalled {} executable{s}: {}", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are you opposed to using "executable" for user-facing copy?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It feels a little weird, I don't have any great alternatives though. "tool entry points"? eh.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We use tool entry points everywhere else in this output, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But entry point is not as general... We also support data scripts which aren't entry points. I feel like entry point is sort of an implementation detail.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fair. Why not just "commands" then?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't have strong feelings here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cargo uses executable
, e.g.
Replacing /Users/zb/.cargo/bin/cargo-nextest
Replaced package `cargo-nextest v0.9.70` with `cargo-nextest v0.9.72` (executable `cargo-nextest`)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, I'm gonna run with executable. I can change other user-facing copy too for consistency.
(I will update fixtures once approved, just a bit tedious.) |
"Installing tool entry points into {}", | ||
"Installing tool entrypoints into: {}", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These are always two words in the official documentation
https://packaging.python.org/en/latest/specifications/entry-points/
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(I like how entrypoints looks too, but I think we should be consistent. I've been following the official docs here)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh strange, thank you, I can change it.
3db5614
to
7507786
Compare
7507786
to
58915da
Compare
Mostly small things. I added entrypoint counts and bolded the executable names.
Closes #4815.